-
Notifications
You must be signed in to change notification settings - Fork 14.3k
KAFKA-18870 Implement describeDelegationToken for controller #19306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
KAFKA-18870 Implement describeDelegationToken for controller #19306
Conversation
A label of 'needs-attention' was automatically added to this PR in order to raise the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Leave some comments.
this.bootstrapControllerOpt = parser.accepts("bootstrap-controller", | ||
"REQUIRED: A comma-separated list of bootstrap.controllers that can be supplied instead of bootstrap-servers." | ||
+ " This is useful for administrators who wish to bypass the brokers.") | ||
.withRequiredArg() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot specify both --bootstrap-server
and --bootstrap-controller
as required. This makes result like:
> ./bin/kafka-delegation-tokens.sh --bootstrap-controller localhost:9093 --describe
Missing required argument "[bootstrap-server]"
Please using withOptionalArg
and do the check in checkArgs
.
serverProperties = { | ||
@ClusterConfigProperty(key = DELEGATION_TOKEN_SECRET_KEY_CONFIG, value = "key") | ||
}) | ||
public void testDescribeDelegationTokenWithBootstrapController(ClusterInstance clusterInstance) throws ExecutionException, InterruptedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case is about Admin. We need another test case to test DelegationTokenCommand directly.
BTW, could you add some content to PR description? Thanks. |
@@ -284,7 +294,7 @@ public String hmac() { | |||
|
|||
public void checkArgs() { | |||
// check required args | |||
CommandLineUtils.checkRequiredArgs(parser, options, bootstrapServerOpt, commandConfigOpt); | |||
CommandLineUtils.checkRequiredArgs(parser, options, commandConfigOpt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed https://github.com/apache/kafka/pull/19306/files#r2034702639 here.
Remaining checks can be done by CommandLineUtils.initializeBootstrapProperties
@johnny94 could you please fix the conflicts? |
@chia7712 Thanks. Fixed it. |
This PR includes
describeDelegationToken
APIcontroller,
bootstrap.controllers
andbootstrap.servers
cannot be setat the same time